Fix VB compiler crashes when decoding attributes by Youssef1313 · Pull Request #61537 · dotnet/roslyn

Youssef1313 · 2022-05-26T18:28:24Z

333fred · 2022-05-27T00:03:56Z

src/Compilers/VisualBasic/Portable/Symbols/Attributes/AttributeData.vb

                Case Else
-                    diagnostics.Add(ERRID.ERR_BadAttribute1, If(nodeOpt IsNot Nothing, nodeOpt.ArgumentList.Arguments(0).GetLocation(), NoLocation.Singleton), Me.AttributeClass)
+                    diagnostics.Add(ERRID.ERR_BadAttribute1,
+                    If(nodeOpt IsNot Nothing, If(nodeOpt.ArgumentList IsNot Nothing AndAlso nodeOpt.ArgumentList.Arguments.Count > 0, nodeOpt.ArgumentList.Arguments(0).GetLocation(), nodeOpt.GetLocation()), NoLocation.Singleton),


Can these still use the helper, even if they only care about the location?

@333fred Do I need to share the helper among classes too? e.g, make it publicly available from AttributeData

Youssef1313 · 2022-05-27T08:07:29Z

src/Compilers/VisualBasic/Portable/Symbols/Attributes/AttributeData.vb

                        Dim argSyntaxLocation As Location = If(arguments.AttributeSyntaxOpt IsNot Nothing,
                                                               arguments.AttributeSyntaxOpt.ArgumentList.Arguments(1).GetLocation(),
                                                               NoLocation.Singleton)


I wasn't able to write a test that makes a crash here, so I didn't make a change.
In case someone was able to, let me know.

Youssef1313

I believe there can be other crashes. Here are code paths I didn't review:

roslyn/src/Compilers/VisualBasic/Portable/Symbols/Source/SourceModuleSymbol.vb

Line 1099 in 9c4920a

    
           DirectCast(arguments.Diagnostics, BindingDiagnosticBag).Add(ERRID.ERR_BadAttribute1, arguments.AttributeSyntaxOpt.ArgumentList.Arguments(0).GetLocation(), attrData.AttributeClass)

roslyn/src/Compilers/VisualBasic/Portable/Symbols/Source/SourceFieldSymbol.vb

Line 733 in 9c4920a

    
           diagnostics.Add(ERRID.ERR_BadAttribute1, arguments.AttributeSyntaxOpt.ArgumentList.Arguments(0).GetLocation(), attrData.AttributeClass)

roslyn/src/Compilers/VisualBasic/Portable/Symbols/Source/SourceAssemblySymbol.vb

Line 1136 in 9c4920a

    
           diagnostics.Add(ERRID.ERR_BadAttribute1, If(nodeOpt IsNot Nothing, nodeOpt.ArgumentList.Arguments(i).GetLocation(), NoLocation.Singleton), attrData.AttributeClass)

#Fixed

AlekseyTs · 2022-05-27T20:58:33Z

Do we have similar issues with C# compiler? At least we probably should add similar tests.

AlekseyTs · 2022-05-27T21:03:17Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.vb

@@ -3763,15 +3821,86 @@ end class

            Dim comp = CompilationUtils.CreateCompilationWithMscorlib40AndVBRuntime(source)
            comp.VerifyDiagnostics(Diagnostic(ERRID.ERR_OmittedArgument2, "FileIOPermission").WithArguments("action", "Public Overloads Sub New(action As System.Security.Permissions.SecurityAction)"),


VerifyDiagnostics

Please use AssertTheseDiagnostics instead. Otherwise, we cannot tell how the error message is going to look like. #Closed

I used it for new tests, but makes sense to use in this modified existing test too. Will do.

AlekseyTs · 2022-05-27T21:04:51Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.vb

@@ -3812,12 +3941,12 @@ end class

            Dim comp = CompilationUtils.CreateCompilationWithMscorlib40AndVBRuntimeAndReferences(source)
            comp.VerifyDiagnostics(


VerifyDiagnostics

AssertTheseDiagnostics #Closed

Youssef1313 · 2022-05-27T21:06:42Z

Do we have similar issues with C# compiler? At least we probably should add similar tests.

I think C# doesn't have the bug, but I'll double check and add tests.

AlekseyTs · 2022-05-27T21:15:58Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.vb

+
+            Dim comp = CompilationUtils.CreateCompilationWithMscorlib40AndVBRuntime(source)
+            comp.AssertTheseDiagnostics(<errors><![CDATA[
+BC40000: 'RequestMinimum' is obsolete: 'Assembly level declarative security is obsolete and is no longer enforced by the CLR by default. See http://go.microsoft.com/fwlink/?LinkID=155570 for more information.'.


BC40000

Please get rid of all unrelated diagnostics from the baselines (applies to other added tests too). For example, it should be possible to disable warnings with #Disable Warning directive #Closed

AlekseyTs · 2022-05-27T21:28:28Z

src/Compilers/VisualBasic/Portable/Symbols/Attributes/AttributeData.vb

-                                        If(nodeOpt IsNot Nothing, nodeOpt.ArgumentList.Arguments(0).GetLocation(), NoLocation.Singleton),
-                                        If(nodeOpt IsNot Nothing, nodeOpt.ArgumentList.Arguments(0).ToString(), ""))
+                        ' BC31215: SecurityAction value '{0}' is invalid for PrincipalPermission attribute
+                        Dim valueLocation = GetArgumentAndLocation(nodeOpt, securityAction)


Dim

Please spell out the type. We are not using implicit typing in declarations when the type isn't obvious from the initial value (i.e. not somehow spelled out there). #Closed

AlekseyTs · 2022-05-27T21:31:48Z

src/Compilers/VisualBasic/Portable/Symbols/Attributes/AttributeData.vb

+            Return GetArgumentAndLocation(nodeOpt, 0).Location
+        End Function
+
+        Private Shared Function GetArgumentAndLocation(nodeOpt As AttributeSyntax, value As Integer) As (Argument As String, Location As Location)


GetArgumentAndLocation

GetFirstArgumentDisplayAndLocation? #Closed

AlekseyTs · 2022-05-27T21:32:20Z

src/Compilers/VisualBasic/Portable/Symbols/Attributes/AttributeData.vb

+            Return GetArgumentAndLocation(nodeOpt, 0).Location
+        End Function
+
+        Private Shared Function GetArgumentAndLocation(nodeOpt As AttributeSyntax, value As Integer) As (Argument As String, Location As Location)


Argument

ArgumentDisplay? #Closed

AlekseyTs · 2022-05-27T21:34:13Z

src/Compilers/VisualBasic/Portable/Symbols/Attributes/AttributeData.vb

            Return Nothing
        End Function

+        Private Shared Function GetLocationForAttributeDiagnostic(nodeOpt As AttributeSyntax) As Location


GetLocationForAttributeDiagnostic

GetFirstArgumentLocation? #Closed

AlekseyTs · 2022-05-27T21:40:05Z

src/Compilers/VisualBasic/Portable/Symbols/Attributes/AttributeData.vb

-                                        If(nodeOpt IsNot Nothing, nodeOpt.ArgumentList.Arguments(0).GetLocation(), NoLocation.Singleton),
-                                        If(nodeOpt IsNot Nothing, nodeOpt.ArgumentList.Arguments(0).ToString(), ""))
+                        ' BC31215: SecurityAction value '{0}' is invalid for PrincipalPermission attribute
+                        Dim valueLocation = GetArgumentAndLocation(nodeOpt, securityAction)


valueLocation

displayAndLocation? #Closed

AlekseyTs · 2022-05-27T21:45:01Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceAssemblySymbol.vb

+                    Else
+                        location = NoLocation.Singleton
+                    End If
+                    diagnostics.Add(ERRID.ERR_BadAttribute1, If(nodeOpt IsNot Nothing, location, NoLocation.Singleton), attrData.AttributeClass)


If(nodeOpt IsNot Nothing,

Do we still need to keep the conditional? #Closed

AlekseyTs · 2022-05-27T21:51:26Z

Done with review pass (commit 3)

Youssef1313 · 2022-06-07T05:37:26Z

It looks like there are already C# tests:

roslyn/src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_Security.cs

Lines 1656 to 1707 in d46613c

    
                   [Fact] 
        
                   [WorkItem(1034429, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1034429")] 
        
                   public void CrashOnParamsInSecurityAttribute() 
        
                   { 
        
                       const string source = @" 
        
           using System.Security.Permissions; 
        
           class Program 
        
           { 
        
               [A(SecurityAction.Assert)] 
        
               static void Main() 
        
               { 
        
               } 
        
           } 
        
           public class A : CodeAccessSecurityAttribute 
        
           { 
        
               public A(params SecurityAction a) 
        
               { 
        
               } 
        
           }"; 
        
                       CreateCompilationWithMscorlib46(source).GetDiagnostics(); 
        
                   } 
        
                   [Fact] 
        
                   [WorkItem(1036339, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1036339")] 
        
                   public void CrashOnOptionalParameterInSecurityAttribute() 
        
                   { 
        
                       const string source = @" 
        
           using System.Security.Permissions; 
        
           [A] 
        
           [A()] 
        
           class A : CodeAccessSecurityAttribute 
        
           { 
        
               public A(SecurityAction a = 0) : base(a) 
        
               { 
        
               } 
        
           }"; 
        
                       CreateCompilationWithMscorlib46(source).VerifyDiagnostics( 
        
                           // (4,2): error CS7049: Security attribute 'A' has an invalid SecurityAction value '0' 
        
                           // [A] 
        
                           Diagnostic(ErrorCode.ERR_SecurityAttributeInvalidAction, "A").WithArguments("A", "0").WithLocation(4, 2), 
        
                           // (5,2): error CS7049: Security attribute 'A' has an invalid SecurityAction value '0' 
        
                           // [A()] 
        
                           Diagnostic(ErrorCode.ERR_SecurityAttributeInvalidAction, "A()").WithArguments("A", "0").WithLocation(5, 2), 
        
                           // (6,7): error CS0534: 'A' does not implement inherited abstract member 'SecurityAttribute.CreatePermission()' 
        
                           // class A : CodeAccessSecurityAttribute 
        
                           Diagnostic(ErrorCode.ERR_UnimplementedAbstractMethod, "A").WithArguments("A", "System.Security.Permissions.SecurityAttribute.CreatePermission()").WithLocation(6, 7)); 
        
                   } 
        
               } 
        
           }

AlekseyTs · 2022-06-09T02:27:38Z

It looks like there are already C# tests

Do they cover all the attributes that we added tests for in this PR for VB? If not, let's open an issue to follow up with tests for C#, listing specific new VB unit-tests. I( do not want to hold this PR because of that.

AlekseyTs

LGTM (commit 5)

AlekseyTs · 2022-06-09T02:29:27Z

@333fred, @dotnet/roslyn-compiler For the second review on a community PR.

Youssef1313 · 2022-06-09T05:43:08Z

It looks like there are already C# tests

Do they cover all the attributes that we added tests for in this PR for VB? If not, let's open an issue to follow up with tests for C#, listing specific new VB unit-tests. I( do not want to hold this PR because of that.

They all share the same code path, but I agree the tests are good to have. I opened #61794.

AlekseyTs · 2022-06-09T17:57:27Z

@dotnet/roslyn-compiler For the second review on a community PR.

AlekseyTs · 2022-06-10T16:00:15Z

@dotnet/roslyn-compiler For the second review on a community PR.

cston · 2022-06-10T16:51:18Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceAssemblySymbol.vb

+                        End If
+                    Else
+                        location = NoLocation.Singleton
+                    End If


Is this the same code used in other Source*Symbol methods? If so, consider extracting a helper method.

cston · 2022-06-10T16:56:05Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.vb

+                                 <![CDATA[
+Imports System.Runtime.InteropServices
+
+<Assembly: TypeLibVersionAttribute(1, -1)> ' Not valid. major is negative.


major

"minor"?

AlekseyTs

LGTM (commit 7)

AlekseyTs · 2022-06-13T15:47:17Z

@Youssef1313 Thanks for the contribution

Fix VB compiler crashes when decoding attributes

05c5780

Youssef1313 requested a review from a team as a code owner May 26, 2022 18:28

ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers labels May 26, 2022

333fred reviewed May 27, 2022

View reviewed changes

Simplify per review comment

69ac80e

Youssef1313 commented May 27, 2022

View reviewed changes

More crash fixes

8d30980

AlekseyTs reviewed May 27, 2022

View reviewed changes

Youssef1313 added 2 commits June 7, 2022 07:57

Address review comments

469bb32

Update AttributeTests_StructLayout.vb

555b2ce

Youssef1313 requested review from 333fred and AlekseyTs June 7, 2022 08:57

AlekseyTs approved these changes Jun 9, 2022

View reviewed changes

Youssef1313 mentioned this pull request Jun 9, 2022

Port few VB attribute unit tests to C# #61794

Open

cston reviewed Jun 10, 2022

View reviewed changes

Youssef1313 added 2 commits June 12, 2022 16:46

Refactor

f702ee7

Fix typo

61a1e1f

Youssef1313 requested a review from cston June 12, 2022 14:46

cston approved these changes Jun 12, 2022

View reviewed changes

AlekseyTs approved these changes Jun 13, 2022

View reviewed changes

AlekseyTs enabled auto-merge (squash) June 13, 2022 13:46

AlekseyTs merged commit 9f883da into dotnet:main Jun 13, 2022

ghost added this to the Next milestone Jun 13, 2022

Youssef1313 deleted the vb-crashes branch June 13, 2022 15:04

RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022

		@@ -3763,15 +3821,86 @@ end class

		Dim comp = CompilationUtils.CreateCompilationWithMscorlib40AndVBRuntime(source)
		comp.VerifyDiagnostics(Diagnostic(ERRID.ERR_OmittedArgument2, "FileIOPermission").WithArguments("action", "Public Overloads Sub New(action As System.Security.Permissions.SecurityAction)"),

		@@ -3812,12 +3941,12 @@ end class

		Dim comp = CompilationUtils.CreateCompilationWithMscorlib40AndVBRuntimeAndReferences(source)
		comp.VerifyDiagnostics(

Conversation

Youssef1313 commented May 26, 2022

Uh oh!

333fred May 27, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 May 27, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 May 27, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 27, 2022

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 May 27, 2022

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented May 27, 2022

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 27, 2022

Uh oh!

Youssef1313 commented Jun 7, 2022

Uh oh!

AlekseyTs commented Jun 9, 2022

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 9, 2022

Uh oh!

Youssef1313 commented Jun 9, 2022

Uh oh!

AlekseyTs commented Jun 9, 2022

Uh oh!

AlekseyTs commented Jun 10, 2022

Uh oh!

cston Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

cston Jun 10, 2022

Youssef1313 left a comment •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading

AlekseyTs May 27, 2022 •

edited

Loading